feat(aggexec): add approx_percentile aggregate function for p95/p99 - #25881
Conversation
The expected result had column `a` typed as a[8,54,0] (int64), but the CI environment maps INT to a[4,32,0] (int32). Regenerated the .result to match the actual CI output.
Resolve conflicts in function_id.go and function_id_test.go: - Keep all new function IDs from main (GET_LOCK, S2, H3, ST_POINT, CAST_STRICT) - Place APPROX_PERCENTILE=542 before FUNCTION_END_NUMBER=543
…centile-quantile-agg # Conflicts: # pkg/sql/plan/function/function_id_test.go
…centile-quantile-agg # Conflicts: # pkg/sql/plan/function/function_id_test.go
|
@fengttt The aggregate/window registration layer was removed in f935280. |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Reviewed exact head 8cbe5fe and refreshed all PR discussion. The author’s latest registration-cleanup statement is supported: runtime RegisterXXX machinery is gone and all 38 fixed executor IDs are pinned against catalog IDs. The aggregate-state reuse fix is correct, current-head CI is green, and no new actionable regression remains. Local focused tests could not start because the worktree lacks prebuilt cgo/libmo.dylib; the worktree remains clean.
|
Fixed the direct DECIMAL128 helper inconsistency in
Added direct-helper regressions for:
Validation on the complete final diff:
@gouhongshen @fengttt please re-review the current head. |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Reviewed exact head 12c0bcd against origin/main after loading all existing reviews, issue comments, inline discussions, and the latest author reply. The new DECIMAL128 helper fix is consistent with aggregate return-type handling and has targeted boundary tests. Previously reported correctness, memory-accounting, registration, and state-reuse issues are resolved. Exact-head CI is green. Focused local tests could not start because the required prebuilt cgo/libmo.dylib is absent; no dependencies were modified.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
One blocking partial-aggregation merge panic remains; the previously reviewed lifecycle and DECIMAL issues are addressed.
P1 - Avoid indexing a missing parity level during partial sketch merge (pkg/sql/colexec/aggexec/approx_percentile.go:115)
A 400-row source sketch compacts level 0 to an empty slice and toggles other.parity[0]. When it is merged into a newly created destination group, appendValues(0, empty) returns before ensureLevel (lines 148-152), so s.parity is still empty and line 116 panics. This occurs in distributed/partial aggregation whenever the merge-side group has no local values. The merge tests populate the destination before merging, so they do not cover this shape.
|
Addressed the current-head partial-merge panic in
Validation:
|
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The upstream merge preserves the prior correctness fixes and resolves the new function-ID collision consistently. No blocking regression remains; one non-blocking memory-efficiency issue persists.
P2 - Allocate sketches lazily for groups containing values (pkg/sql/colexec/aggexec/approx_percentile.go:570)
GroupGrow eagerly creates a quantileSketch for every group, including high-cardinality groups whose aggregate argument is always NULL. Fill, BulkFill, BatchFill, and merge already allocate lazily, while Flush explicitly handles nil state, so these empty sketches are unnecessary. Their Go-managed objects and slice headers are also excluded from the group spill calculation, which only reads ctr.mp.CurrNB(); consequently an all-NULL high-cardinality aggregation can accumulate substantial untracked heap memory. Remove this loop and retain ensureSketch at the existing value/merge sites.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Prior blocking review findings are resolved at fa762ce. One non-blocking memory-efficiency issue remains.
P2 - Allocate sketches lazily for NULL-only groups (pkg/sql/colexec/aggexec/approx_percentile.go:570)
GroupGrow eagerly creates a quantileSketch for every group. Fill/BatchFill skip NULL inputs before ensureSketch, merge skips nil source state, and Flush already emits NULL for a nil sketch, so groups whose value is always NULL never need one. These per-group Go heap objects and slice headers are outside mpool accounting; high-cardinality NULL-only aggregation therefore adds avoidable untracked heap pressure. Remove this eager loop and retain the existing lazy creation at value/merge sites.
…7-approx-percentile # Conflicts: # pkg/sql/plan/function/function_id.go # pkg/sql/plan/function/function_id_test.go
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Prior blockers for mpool lifecycle, configuration propagation, numeric/DECIMAL precision, registration removal, state replacement, and partial merges are fixed. The prior lazy NULL-only sketch allocation remains unchanged and non-blocking. A non-finite float endpoint failure is blocking.
P1 - Return extrema for infinite float inputs (pkg/sql/colexec/aggexec/approx_percentile.go:925)
Author response: non-finite inputs were intentionally left with the prior IEEE-754 propagation behavior. Why this remains blocking: non-finite float values are not excluded—CAST('Inf' AS DOUBLE) is accepted by the float cast path—and this aggregate accepts float64. For approx_percentile(CAST('Inf' AS DOUBLE), 1), Quantile returns lo=hi=+Inf with zero fraction, but interpolateFloat64 checks for Inf before the zero-fraction case and evaluates +Inf + (+Inf - +Inf) * 0, producing NaN. Thus p=1 fails to return the maximum; p=0 and singleton inputs have the same defect. Move the zero-fraction/equal-endpoint handling before the non-finite branch, and add executor/SQL coverage for +/-Inf endpoints and singleton inputs.
|
Fixed the non-finite endpoint issue in 07d1efd.
Added coverage for:
Validation passed: full |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The previously blocking non-finite endpoint issue is fixed: exact ranks and equal infinite endpoints now bypass invalid infinity arithmetic, with executor, merge/serialization, and SQL regressions. Earlier blockers remain resolved. The unchanged eager allocation for NULL-only groups remains accepted as non-blocking and is not repeated in this follow-up.
Preserve aggregate-state cleanup alongside main's single-chunk reuse and make infinity BVT assertions compatible with mo-tester.
# Conflicts: # pkg/sql/compile/operator.go
|
Merged the latest |
# Conflicts: # pkg/sql/plan/function/function_id.go # pkg/sql/plan/function/function_id_test.go
Merge Queue Status
This pull request spent 1 hour 3 minutes 48 seconds in the queue, with no time running CI. ReasonPull request #25881 has been dequeued Queue conditions are not satisfied:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
# Conflicts: # pkg/sql/compile/operator.go # pkg/sql/plan/base_binder.go
Merge Queue Status
This pull request spent 2 hours 22 minutes 49 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Requeued — the merge queue status continues in this comment ↓. |
# Conflicts: # pkg/sql/plan/function/function_id_test.go
Merge Queue Status
This pull request spent 21 minutes 39 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
8 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24550
What this PR does / why we need it:
This PR continues the unfinished work from #24667 while preserving its original commits and authorship.
It adds approx_percentile(value, percentile) with:
Review-blocker regressions cover 10K rows, duplicate values, NULLs, four-way partial merge, intermediate-state round trips, replacement of reused executors with empty and multi-chunk state, state-size bounds, mismatched merge configuration, and DECIMAL64/128 values above 2^53.
Validation